BaseControl: Add opt-in prop for margin-free styles #39325
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mirka
commented
Mar 9, 2022
| export const StyledHelp = styled.p` | ||
| margin-top: ${ space( 2 ) }; |
There was a problem hiding this comment.
We can go ahead and add this margin-top regardless of whether StyledField still has a margin-bottom, because this margin-top will collapse together with StyledField's margin-bottom anyway.
49 tasks
|
Size Change: +66 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
ciampo
approved these changes
Mar 10, 2022
| <StyledField className="components-base-control__field"> | ||
| <StyledField | ||
| className="components-base-control__field" | ||
| // TODO: Official deprecation for this should start after all internal usages have been migrated |
There was a problem hiding this comment.
Official deprecation for this should start after all internal usages have been migrated
👍
| @@ -15,13 +14,14 @@ export default { | |||
| const BaseControlWithTextarea = ( { id, ...props } ) => { | |||
| return ( | |||
| <BaseControl id={ id } { ...props }> | |||
| <TextareaControl id={ id } /> | |||
| <textarea style={ { display: 'block' } } id={ id } /> | |||
There was a problem hiding this comment.
Is there a particular reason why we'd swap the library's component with the vanilla HTML one? Do you think this should be a pattern that we apply to stories across the package?
There was a problem hiding this comment.
Haha so when I was working on this PR there was a weird issue I was debugging for half an hour, and I finally realized that TextareaControl is already built with BaseControl 🤦🏻♀️ 15095fb
| return ( | ||
| ! __nextHasNoMarginBottom && | ||
| css` | ||
| margin-bottom: revert; |
There was a problem hiding this comment.
TIL (or rediscovered) revert — very cool!
| export const StyledHelp = styled.p` | ||
| margin-top: ${ space( 2 ) }; |
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
20 tasks
mirka
added a commit
that referenced
this pull request
Mar 14, 2022
Also fixes a conflict resolution mishap in #39325
mirka
added a commit
that referenced
this pull request
Mar 14, 2022
* Divider: Display inline by default when orientation is vertical * Improve type docs for `orientation` * Improve stories * Add changelog entry * Simplify prop types * Fixup changelog Also fixes a conflict resolution mishap in #39325
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part 1 of #38730
Prerequisite for #38720
What?
Adds a
__nextHasNoMarginBottomprop so consumers can opt-in to the new styles without outer margins.Why?
As described in #38730, components that include outer margins are hard to reuse in different contexts.
How?
Generally follows the style deprecation procedure in the docs, except we aren't formally deprecating the margin styles yet. We should initiate the formal deprecation (i.e. trigger a deprecation warning) only after all internal usages have been migrated.
Testing Instructions
npm run storybook:devand check out the stories forBaseControl.__nextHasNoMarginBottomistrue, the component should have no outer bottom margin. (Regardless of whether it includes ahelptext.)